GH-46179: [Python] Bump index level once if pandas df already contains __index_level_i__ column - #46884
Conversation
|
|
c915159 to
3ee4599
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses GH-46179 in PyArrow’s pandas conversion by avoiding duplicate Arrow field names when a pandas DataFrame already contains __index_level_i__ columns, ensuring generated index columns use a non-conflicting name.
Changes:
- Update generated index column naming to pick the next available
__index_level_{j}__name if the default collides with existing columns. - Ensure uniqueness across both DataFrame columns and previously generated index columns when multiple index levels are serialized.
- Add regression tests for single-index and MultiIndex cases where
__index_level_0__already exists as a DataFrame column.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/pyarrow/pandas_compat.py | Adjusts index-level name generation to avoid collisions with existing column names and previously assigned index column names. |
| python/pyarrow/tests/test_pandas.py | Updates existing metadata assertion and adds new regression tests validating the bumped index column names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| j = i | ||
| while f'__index_level_{j:d}__' in column_names: | ||
| j += 1 | ||
| return f'__index_level_{j:d}__' |
There was a problem hiding this comment.
Isn't schema based conversion already buggy without this change when it comes to the index levels? It probably silently ignores the duplicated level 0 currently?
There was a problem hiding this comment.
OK, getting used to this :) Copilot can't answer. Well, I think the change suggested can be a possible follow-up if we see this would be needed. But I do not think it is in the scope of this PR.
|
@jorisvandenbossche what do you think of the proposed change in this PR? |
|
cc @raulcd if you have some time for review. No rush! |
raulcd
left a comment
There was a problem hiding this comment.
To be fair I don't know much about the __index_level_{i} works.
Can we add an explicit test to validate the index is respected?
Based on the issue snippet:
In [40]: df = pd.DataFrame({"col": [1, 2, 3], "__index_level_0__": [1, 2, 3]}, index=[2, 3, 4])
In [41]: df
Out[41]:
col __index_level_0__
2 1 1
3 2 2
4 3 3
In [42]: pa.table(df)
Out[42]:
pyarrow.Table
col: int64
__index_level_0__: int64
__index_level_0__: int64
----
col: [[1,2,3]]
__index_level_0__: [[1,2,3]]
__index_level_0__: [[2,3,4]]
I would expect the pa.table(df).to_pandas() to continue presenting the index correctly:
>> pa.table(df).to_pandas()
col __index_level_0__
2 1 1
3 2 2
4 3 3
Can we validate the pandas index is the new __index_level_1__ instead of the column __index_level_0__?
|
Dev failure does not seem to be connected to the changes in this PR. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a81e6c6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
PyArrow adds a
__inex_level_i__column to the PyArrow table if the pandas dataframe has an unnamed index it wants to preserve. Currently that creates a duplicate, if such a column already exists.What changes are included in this PR?
Bumping the integer number in the generated column in order to not get any duplicates.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.